feat: Limit templates to defined file extension #1990
Conversation
- Defaults to ‘html’ unless overriden by config setting `template_extension` - Allows use of `.erb` processed files such as CSS or JavaScript to be in sidecar location with HTML template(s) Signed-off-by: Fred Boyle <vortex+github@snark.one>
|
@boardfish Checking in to see if I could get assistance in completing this and getting the code reviewed and eventually merged in. Thanks. |
|
@fredboyle thanks for your PR. Might you provide docs and/or tests that necessitate this change? |
@joelhawksley Gladly. Would you kindly point me to the best location for tests? It will be config variation testing and invalid path tests. However it's unclear to me where these tests live as well as how to run the tests in this repo. Thanks for your guidance and help. |
|
@fredboyle I'd look at existing tests and see what you think is the closest fit. Perhaps we might be better off setting this at the component level though? I could see setting it on ApplicationComponent. |
Ah a good point regarding the component level configuration instead of system wide. Was not aware that was part of the architecture. I'll investigate and update the pull-request. |
Add test and supporting content Signed-off-by: Fred Boyle <vortex+github@snark.one>
Signed-off-by: Fred Boyle <vortex+github@snark.one>
Add user documentation Update test example files Signed-off-by: Fred Boyle <vortex+github@snark.one>
|
@joelhawksley Updated things to be more flexible with sensible defaults and added a test. Please take a look and let me know if this fits into the ViewComponent approaches. Thanks. |
There was a problem hiding this comment.
@fredboyle thanks for writing this PR!
After giving it a read, I'm wondering if what we really need to do is allow folks to explicitly set the format for a component. If we did, you could have multiple ERB files and VC would just use the one for the selected format, which is currently HTML by default. What do you think about going that direction?
|
@joelhawksley This is what this pull-request permits a user to do. By default ViewComponent scans the files for each handler, the default handler being ERB if it finds more than one file it reports too many templates were found. It doesn't distinguish between file types within a handler. Unless I missed something, possible since I'm not super familiar with this codebase. ViewComponent defaults to rendering HTML as the template but only if no errors were triggered by the handler file search. The code in this pull-request allows a user to have as many files as needed per handler without triggering the error AND also set which file type will be used to render the component. So there could be Hoping my understanding is correct and my explanation makes sense. Glad to hop on a live call if helpful to sync up and discuss. |
@fredboyle thank you for your explanation! Giving this some more thought, I wonder if we should change the underlying behavior. I'd rather see us warn if there are multiple templates per format and/or variant and not worry about multiple templates per extension, assuming only one is HTML. What do you think about that approach? |
@joelhawksley Hmm not certain I'm clear on what you mean here. When you say format do you mean a handler such as ERB or HAML? If so then that's the problem I'm attempting to solve for. Multiple per handler causes issues if, for example, a component has both the HTML template to be rendered AND say the CSS sidecar file being processed by ERB. I do tend to agree that HTML is the template file type for nearly all components. It may deviate from that standard but would be a rare exception. In my experience it would mostly be the handler/processor that changes and not the HTML nature of the template. So we could lock that down to only scan for HTML files regardless of handler. If we take that approach then we no longer need to have a component configuration and we simply reduce the scope of what ViewComponent considers to be a template. In this case a Does that make sense? Are we saying the same thing? Thanks again for your help in working through this. |
|
|
||
| ## Setting template file extension | ||
|
|
||
| Sometimes a component will warrant the use of multiple file types for a single template handler. An example would be ERB handled files for both HTML and CSS. By default ViewComponent will return a "More than one template found for" error. To avoid this set the `VC_TEMPLATE_EXTENSION` constant in your component to the file extension of the template you wish your component to use. |
There was a problem hiding this comment.
Is there a reason we wouldn't want to change the existing logic to look for multiple extensions and unique by that?
There was a problem hiding this comment.
@BlakeWilliams I'm not clear on why a component would need multiple template file extensions. Could you provide an example to help me understand?
My understanding and experience is that a component renders a single type of output, usually HTML. So the variant rules apply to that HTML file type/extension if varying outputs are needed.
There was a problem hiding this comment.
I don't see why a component would care if there's both an HTML and a CSS file associated with it. The only case I think we care about is having two of the same filetype, like foo.html.erb and foo.html.haml, for example.
We also already have a way to determine the type of content a component should be rendering, via format
view_component/lib/view_component/base.rb
Lines 593 to 595 in 5ae3c12
There was a problem hiding this comment.
@BlakeWilliams We're on the same page. The component should only care about what you mentioned. However the current functionality starts with the handler/processor and not the filetype such as .html.*.
Hence why there's a need to add a bit of code to limit the scope. This is to avoid the errors that arise if a component has more than one filetype per handler/processor.
Regarding the format at the class level you pointed out I did see that but I'm not certain it does much currently. However it doesn't mean that it shouldn't be leveraged in the solution we're aiming for. So tweaking things to use it is likely a good idea.
|
Checking in to see if there are any further thoughts on the discussions. |
|
@BlakeWilliams do you have any further feed back for @fredboyle so we can maybe get some progress on this |
What are you trying to accomplish?
template_extension.erbprocessed files such as CSS or JavaScript to be in sidecar location with HTML template(s)template_extensionconfig setting to specify a custom file extension to use as the template.css.erbsidecar file for component #1783Anything you want to highlight for special attention from reviewers?
Unclear how to properly run tests for this repo. Clarification from someone with more intimate knowledge of this repo would be appreciated.